Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

hover: Include struct and interface fields #169

Merged
merged 1 commit into from
Mar 1, 2017
Merged

hover: Include struct and interface fields #169

merged 1 commit into from
Mar 1, 2017

Conversation

keegancsmith
Copy link
Member

For structs and interfaces we now include an extra hover content field containing the fields in the struct.

image

image

Fixes #168

@keegancsmith keegancsmith merged commit 23a59cc into master Mar 1, 2017
@keegancsmith keegancsmith deleted the k/hover branch March 1, 2017 17:32
@junkblocker
Copy link

junkblocker commented Mar 1, 2017

The original display shows raw/original strings as it is which is better than the sourcegraph which shows them as cooked strings. See:

Original version:
https://gyazo.com/f16dc38308c76fe0997a0172f485bc09

Sourcegraph version:
https://gyazo.com/82df5716b5fb15e0bcbbb8f0af417b32

Though it is cool that the Sourcegraph version also shows godoc.

@keegancsmith
Copy link
Member Author

@junkblocker shouldn't you just peak definition for that?

@junkblocker
Copy link

@keegancsmith, yes but that's irrelevant to comparing current vs new implementation hovertip views.

@keegancsmith
Copy link
Member Author

@ramya-rao-a is the goal for us to be exactly the same as the older version? I personally prefer the new implementation for hover vs what vscode-go used to do (in fact I also want to get rid of the tags). But I'm happy to make it more like the old implementation if that is what makes sense.

Also in future, if we get the signature helper to work on structs I suppose that would help the common reason people look at this info?

@grobie
Copy link

grobie commented Mar 2, 2017

I also prefer the new implementation.

@junkblocker
Copy link

@keegancsmith , @ramya-rao-a - Can't this be a switch? I'd prefer to see both the uncooked display and the struct tags.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Mar 2, 2017

There can always be cases where a feature implementation by the language server is different than that of what vscode-go has at the moment. It might also be better. So no, the goal is not to "exactly" match the existing behavior. But it should match existing needs.

In the current case, having the docs squeezed in between the struct type and struct members does look a bit odd, but that's my personal opinion.

I'd say, lets keep it as is, and wait for more feedback from users.

This would also affect hover in source graph, do you (at source graph) have any preference?

@ramya-rao-a
Copy link
Contributor

It would be nice to have a poll :)

@sqs
Copy link
Member

sqs commented Mar 3, 2017

Making the struct tags raw strings is definitely better (Foo string \json:"foo"`instead ofFoo string "json:"foo""`). We'll definitely do that. I think that's a no-brainer.

As for showing the doc synopsis (first sentence), Sourcegraph users generally do like seeing that in addition to the struct definition. (We are totally fine having different behavior on Sourcegraph vs. VSCode, of course.) The improvement we should make is to display the doc prose in a sans-serif font, not in monospace. That'll better distinguish the doc synopsis from the struct definition, and it'll make the doc synopsis take up less space.

Feel free to upvote this if you like seeing doc synopses and downvote if you do not. 😄

@ramya-rao-a
Copy link
Contributor

@sqs Just to be clear, when you say "doc synopsis" you mean the first line and "doc prose" you mean the second correct?

image

@sqs
Copy link
Member

sqs commented Mar 3, 2017 via email

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Mar 3, 2017

We should definitely include the doc string. No arguments there.

Regarding the font issue, what is happening is the language server is returning a markdown content for the hover and not simple text. See

doc.ToMarkdown(&b, comments, nil)
.

Simple text is shown in a smaller font (you can see the current behavior in VS Code), markdown for some reason appears bigger and different font. I'll take a look at that separately from VS Code side.

Things left to debate/discuss/vote/poll are

  • Show raw original strings vs cooked strings for the members
  • Include the members in the first block right with the type definition followed by the doc string in the second block (current behavior in VS Code) or the members appear after the doc string in the third block (behavior shown in the description of this issue)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants